Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

Fix issues with deploy:Registrar and deploy:AngelProtocol #215

Merged
merged 10 commits into from
Jul 21, 2023

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Jul 20, 2023

Explanation of the solution

  • missing IndexFund > registrarContract update
  • missing GasFwdFactory > registrar update
  • admin of GasFwdFactory should be proxyAdmin (currently deployer)
  • LocalRegistrar.updateNetworkConnections should use enum NetworkConnectionAction for its action param

Instructions on making this work

  • run yarn or yarn install to install npm dependencies
  • run yarn test to verify all tests still pass
  • run npx hardhat node
  • run yarn deploy --network localhost --yes
  • verify deployment successful
  • run npx hardhat deploy:Registrar --network localhost --yes
  • verify deployment successful
  • verify Accounts registrar address updated to the newly deployed
  • verify IndexFund registrar address updated to the newly deployed
  • verify GasFwdFactory registrar address updated to the newly deployed

@0xNeshi 0xNeshi added the bug Something isn't working label Jul 20, 2023
@0xNeshi 0xNeshi self-assigned this Jul 20, 2023
@0xNeshi 0xNeshi changed the title Fix issues with deploy:Registrar Fix issues with deploy:Registrar and deploy:AngelProtocol Jul 20, 2023
@@ -71,7 +71,7 @@ task("deploy:AngelProtocol", "Will deploy complete Angel Protocol")

const gasFwd = await deployGasFwd(
{
deployer: deployer,
deployer: proxyAdmin,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be APTeamMultisig. See my longer comment in my PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signer deployer is actually only used to deploy the GasFwd implementation contract (see source), but the contract itself has no dependency on who the admin is (see source).
This means we can remove deployer completely and use admin to deploy GasFwd.

Comment on lines +7 to +11
export enum NetworkConnectionAction {
NONE,
POST,
DELETE,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a type not a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enums in TS are actually objects (like many things in JS/TS it's confusing, I know).
https://www.typescriptlang.org/docs/handbook/enums.html#enums-at-runtime

Since we have many enums we need to create, we can store them in a separate enums.ts file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea! Thanks for the info

@stevieraykatz stevieraykatz merged commit 2967556 into master Jul 21, 2023
1 check passed
@stevieraykatz stevieraykatz deleted the fix-depl-reg branch July 21, 2023 20:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants